Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove long/int relic #37420

Merged
merged 5 commits into from
Mar 31, 2024
Merged

Remove long/int relic #37420

merged 5 commits into from
Mar 31, 2024

Conversation

nbruin
Copy link
Contributor

@nbruin nbruin commented Feb 22, 2024

in py2, long and int were different types. In py3, only int remains, with long a synonym in cython.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@nbruin
Remove long/int relic
81a71ae 
in py2, long and int were different types. In py3, only int remains, with long a synonym in cython.
@videlec
Copy link
Contributor

videlec commented Feb 22, 2024

Wouldn't it make sense to get rid of sage.rings.integer.int_to_Z? (actually I would get rid of it and then rename long_to_Z -> int_to_Z)

@nbruin
remove int_to_Z because it's buggy
33e14a9 
int_to_Z used to work in Py2 on the type "int" there which were integers that fit in a word. In Py3 there is no such type anymore and "int" now is what "long" used to be -- -arbitrary length integers. In Py3 int_to_Z is buggy:

sage: itoZ=sage.rings.integer.int_to_Z()
sage: itoZ(1000000000000000000000)
OverflowError
@nbruin
remove forward declaration of int_to_Z
fa1905c
@nbruin
Copy link
Contributor Author

nbruin commented Feb 22, 2024

Yes, it's worse. int_to_Z still exists but is buggy for what int is in Py3. Removed the definition to see what happens. Will probably rename long_to_Z to int_to_Z in the next step (with that change hopefully we don't have to change any code elsewhere)

@videlec
Copy link
Contributor

videlec commented Feb 22, 2024

Maybe keep an alias long_to_Z = int_to_Z to avoid backward incompatibility?

@nbruin
Copy link
Contributor Author

nbruin commented Feb 22, 2024

well ... int_to_Z was exported for cimportin the pxd but removing that didn't cause any test failures, so I guess it's not used. Indeed, these coercion morphisms should only be used for the coercion system. Any time-critical code can use much more direct conversion routines.

I guess in cython long and int are synonyms (as far as python types are concerned), but in py3, only int remains, so I guess we should rename. Another round of tests will tell us the call sites ...

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 18ace4e; changes) is ready! 🎉

@nbruin
Copy link
Contributor Author

nbruin commented Feb 22, 2024

OK, testing seems successful. Ready to be merged, I think. Review?

grhkm21
grhkm21 approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, cleaning up code is always nice.

@grhkm21
Copy link
Contributor

grhkm21 commented Feb 23, 2024

And I agree that keeping (alias) long_to_Z is not necessary. Looking through the 244 usages of long_to_Z on GitHub I see all of them are just forks on Sage, so it's okay.

Also there's a function PyLong_to_ZZ still in Sage, but that's correct to keep since PyLong is a CPython thing(?)

@nbruin
Copy link
Contributor Author

nbruin commented Feb 23, 2024 •
edited
Loading

Also there's a function PyLong_to_ZZ still in Sage, but that's correct to keep since PyLong is a CPython thing(?)

Yes I think it is:
https://docs.python.org/3/c-api/long.html#c.PyLongObject

tornaria added a commit to tornaria/sage that referenced this pull request Mar 31, 2024
@tornaria
Make this PR mergeable with sagemath#37420
867ece7
@tornaria tornaria mentioned this pull request Mar 31, 2024
Merged
3 tasks
@vbraun vbraun merged commit 4796e74 into sagemath:develop Mar 31, 2024
21 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grhkm21 grhkm21 grhkm21 approved these changes

Assignees
No one assigned
Labels
p: trivial / 5
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
5 participants
@nbruin @videlec @grhkm21 @vbraun @mkoeppe